-
Notifications
You must be signed in to change notification settings - Fork 55
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
materialisation: Added new message fields. #1888
base: main
Are you sure you want to change the base?
Conversation
5ab145b
to
d2c8252
Compare
d2c8252
to
479e926
Compare
479e926
to
598e1cb
Compare
598e1cb
to
d6b573f
Compare
d6b573f
to
d14268c
Compare
858aaad
to
1c0c2ba
Compare
1c0c2ba
to
5bd7dfc
Compare
5bd7dfc
to
e6e9d57
Compare
This was just a mistype in the PR body, the code uses updatedAt and I have now corrected the typo. |
11e045c
to
0506a3c
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 3
🧹 Outside diff range and nitpick comments (1)
src/common/lib/types/message.ts (1)
339-345
: Add JSDoc documentation for new propertiesThe new properties would benefit from clear documentation explaining their purpose and expected values.
Add JSDoc comments for the new properties:
/** The action type of the message */ action?: API.MessageAction | number | undefined; /** Unique identifier including message index */ serial?: string; /** Reference to another message's serial */ refSerial?: string; /** Type of the reference */ refType?: string; /** Timestamp when the message was last updated */ updatedAt?: number; /** Timestamp when the message was deleted */ deletedAt?: number; /** Operation details */ operation?: API.Operation;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
- ably.d.ts (1 hunks)
- src/common/lib/client/restchannel.ts (1 hunks)
- src/common/lib/types/message.ts (7 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/common/lib/client/restchannel.ts
🧰 Additional context used
🪛 Biome
src/common/lib/types/message.ts
[error] 287-288: The assignment should not be in an expression.
The use of assignments in expressions is confusing.
Expressions are often considered as side-effect free.(lint/suspicious/noAssignInExpressions)
🔇 Additional comments (6)
src/common/lib/types/message.ts (2)
12-20
: LGTM!The
MessageActionArray
constant provides a clear enumeration of possible message actions.
22-26
:⚠️ Potential issueImprove array bounds checking and error handling
The function should handle out-of-bounds indices explicitly and return a default value or throw an error for invalid indices.
Apply this change:
function toMessageActionString(actionNumber: number): API.MessageAction { - if (actionNumber in MessageActionArray) { + if (actionNumber >= 0 && actionNumber < MessageActionArray.length) { return MessageActionArray[actionNumber]; } + return 'message_unset'; }Likely invalid or redundant comment.
ably.d.ts (4)
2338-2365
: LGTM! Message interface changes are well-structuredThe new fields added to the Message interface are well-documented and properly typed. The optional nature of these fields is appropriate as they'll only be present in specific message scenarios.
2386-2431
: LGTM! MessageActions namespace is well-structuredThe MessageActions namespace and MessageAction type are well-documented and provide a clear set of possible message actions.
2369-2370
:⚠️ Potential issueFix typo in Operation interface documentation
The comment has a typo: "update of deletion" should be "update or deletion"
Apply this diff:
- * Contains the details of an operation, such as update of deletion, supplied by the actioning client. + * Contains the details of an operation, such as update or deletion, supplied by the actioning client.Likely invalid or redundant comment.
2383-2383
: 🛠️ Refactor suggestionConsider using a more flexible type for metadata
The current type
Record<string, string>
only allows string values, which might be too restrictive for complex metadata. Consider usingRecord<string, unknown>
or a more flexible type to allow for nested objects and different value types.- metadata?: Record<string, string>; + metadata?: Record<string, unknown>;Likely invalid or redundant comment.
0506a3c
to
543b360
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
- src/common/lib/client/restchannel.ts (1 hunks)
- src/common/lib/types/message.ts (7 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/common/lib/client/restchannel.ts
🧰 Additional context used
🪛 Biome
src/common/lib/types/message.ts
[error] 287-288: The assignment should not be in an expression.
The use of assignments in expressions is confusing.
Expressions are often considered as side-effect free.(lint/suspicious/noAssignInExpressions)
🔇 Additional comments (4)
src/common/lib/types/message.ts (4)
12-20
: LGTM!The
MessageActionArray
provides a clear enumeration of valid message actions.
297-305
: LGTM!The
fromValues
function properly handles action conversion with the new options parameter.
339-345
: LGTM!The new message properties are well-implemented:
- Properties are properly typed
- Serialization handles the new fields correctly
- String representation includes all new fields
Also applies to: 376-382, 405-411
312-312
:⚠️ Potential issueAdd type validation before type assertion
The type assertion
as Record<string, unknown>
assumes the input is of the correct type without validation.Apply this change:
- result[i] = fromValues(values[i] as Record<string, unknown>); + const value = values[i]; + if (!value || typeof value !== 'object') { + throw new ErrorInfo('Invalid message value', 40013, 400); + } + result[i] = fromValues(value as Record<string, unknown>);Likely invalid or redundant comment.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
- src/common/lib/types/message.ts (7 hunks)
🧰 Additional context used
🪛 Biome
src/common/lib/types/message.ts
[error] 287-288: The assignment should not be in an expression.
The use of assignments in expressions is confusing.
Expressions are often considered as side-effect free.(lint/suspicious/noAssignInExpressions)
🔇 Additional comments (1)
src/common/lib/types/message.ts (1)
312-312
:⚠️ Potential issueAdd type validation before type assertion
The type assertion
values[i] as Record<string, unknown>
could be unsafe if the input doesn't match the expected type.- result[i] = fromValues(values[i] as Record<string, unknown>); + const value = values[i]; + if (!value || typeof value !== 'object') { + throw new ErrorInfo('Invalid message value', 40013, 400); + } + result[i] = fromValues(value as Record<string, unknown>);Likely invalid or redundant comment.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we add a new test, or update an existing test, that covers the new fields please?
Perhaps a unit test on fromValues
as that's where a lot of the magic happens?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (1)
src/common/lib/types/message.ts (1)
12-20
: Add JSDoc documentation for the message action constantsConsider adding documentation to explain the purpose and usage of these message actions, especially since they're part of the public API.
/** * Array of valid message actions in order of their numeric representation. * Used for converting between action numbers and their string representations. * @internal */ const MessageActionArray: API.MessageAction[] = [ 'message_unset', 'message_create', // ... rest of the actions ];
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
- src/common/lib/types/message.ts (7 hunks)
🧰 Additional context used
🪛 Biome
src/common/lib/types/message.ts
[error] 287-288: The assignment should not be in an expression.
The use of assignments in expressions is confusing.
Expressions are often considered as side-effect free.(lint/suspicious/noAssignInExpressions)
🔇 Additional comments (2)
src/common/lib/types/message.ts (2)
376-382
:⚠️ Potential issueAdd error handling for action conversion in toJSON
The action conversion could throw an error if the action is invalid. Consider handling this case gracefully.
- action: toMessageActionNumber(this.action as API.MessageAction) || this.action, + action: (() => { + try { + return toMessageActionNumber(this.action as API.MessageAction) || this.action; + } catch (error) { + Logger.logAction( + Logger, + Logger.LOG_ERROR, + 'Message.toJSON()', + `Invalid action: ${this.action}`, + ); + return undefined; + } + })(),Likely invalid or redundant comment.
28-34
:⚠️ Potential issueAdd input validation to conversion functions
The conversion functions should validate their inputs to fail fast and provide clear error messages.
function toMessageActionString(actionNumber: number): API.MessageAction | undefined { + if (typeof actionNumber !== 'number' || !Number.isInteger(actionNumber) || actionNumber < 0) { + throw new ErrorInfo('Invalid action number', 40013, 400); + } return ReverseMessageActionMap.get(actionNumber); } function toMessageActionNumber(messageAction?: API.MessageAction): number | undefined { + if (messageAction && !MessageActionMap.has(messageAction)) { + throw new ErrorInfo('Invalid message action', 40013, 400); + } return messageAction ? MessageActionMap.get(messageAction) : undefined; }Likely invalid or redundant comment.
@@ -83,7 +83,8 @@ export function fromDeserialized( | |||
const error = deserialized.error; | |||
if (error) deserialized.error = ErrorInfo.fromValues(error as ErrorInfo); | |||
const messages = deserialized.messages as Message[]; | |||
if (messages) for (let i = 0; i < messages.length; i++) messages[i] = messageFromValues(messages[i]); | |||
if (messages) | |||
for (let i = 0; i < messages.length; i++) messages[i] = messageFromValues(messages[i], { stringifyAction: true }); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(I don't want to comment too much on the coding style of a project I'm not a primary contributor to any more, but is this really the preferred coding style for ably-js these days? No braces for a multiline if
, and the body of a for
on the same line as the condition? Seems an.. unusual choice)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can't really comment on that either as this is my first contribution , but it seems to be the norm throughout. I usually try and follow the coding style of the repo, but in this case I think I'll make the change to something more structured.
- Added new message attributes, including `action`, `serial`, `refSerial`, `refType`, `updatedAt`, `deletedAt`, and `operation`. Additionally, create functions to map message actions between string and number representations. This update also changes the `fromValues` function to handle action transformations.
431e31a
to
e25751e
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 3
🧹 Outside diff range and nitpick comments (3)
test/support/runPlaywrightTests.js (1)
71-84
: Consider documenting the grep functionality.This new filtering capability is a valuable addition to the test infrastructure. Consider:
- Adding a comment block explaining the grep functionality and its usage
- Updating the README or testing documentation to include examples of using
MOCHA_GREP
- Adding examples of common grep patterns for frequently used test scenarios
Example documentation:
/** * Test filtering via MOCHA_GREP * * Usage: * MOCHA_GREP="MyTestSuite" npm test * MOCHA_GREP="should handle.*errors" npm test * * The grep pattern is applied across all browser types and supports * regular expressions for flexible test filtering. */src/common/lib/types/message.ts (1)
12-34
: LGTM! The implementation follows the dot notation format and uses Maps for efficient lookups.The use of Maps for bidirectional conversion provides O(1) lookup time, which is an improvement over array iteration. However, consider adding input validation to prevent runtime errors.
function toMessageActionString(actionNumber: number): API.MessageAction | undefined { + if (typeof actionNumber !== 'number' || !Number.isInteger(actionNumber) || actionNumber < 0) { + throw new ErrorInfo('Invalid action number', 40013, 400); + } return ReverseMessageActionMap.get(actionNumber); } function toMessageActionNumber(messageAction?: API.MessageAction): number | undefined { + if (messageAction && !MessageActionMap.has(messageAction)) { + throw new ErrorInfo('Invalid message action', 40013, 400); + } return messageAction ? MessageActionMap.get(messageAction) : undefined; }test/realtime/message.test.js (1)
1275-1324
: LGTM! Well-structured test suite for message action stringification.The test suite thoroughly covers various scenarios for message action handling:
- Numeric to string conversion
- Pre-stringified actions
- Edge cases (undefined/unknown actions)
- Consistent JSON serialization
The implementation aligns well with the PR objectives for enhancing message action support.
Consider adding the following test cases to improve coverage:
- Invalid action values (e.g., negative numbers, non-numeric strings)
- Boundary values for action enum
- Case sensitivity handling for stringified actions
// Additional test cases { description: 'should handle invalid numeric action', action: -1, options: { stringifyAction: true }, expectedString: '[Message; action=-1]', expectedJSON: { action: -1 } }, { description: 'should handle case-sensitive action strings', action: 'MESSAGE.CREATE', options: { stringifyAction: true }, expectedString: '[Message; action=MESSAGE.CREATE]', expectedJSON: { action: undefined } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (10)
- ably.d.ts (1 hunks)
- package.json (1 hunks)
- scripts/moduleReport.ts (1 hunks)
- src/common/lib/client/restchannel.ts (1 hunks)
- src/common/lib/types/defaultmessage.ts (2 hunks)
- src/common/lib/types/message.ts (7 hunks)
- src/common/lib/types/protocolmessage.ts (1 hunks)
- test/realtime/crypto.test.js (2 hunks)
- test/realtime/message.test.js (2 hunks)
- test/support/runPlaywrightTests.js (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (5)
- package.json
- scripts/moduleReport.ts
- src/common/lib/client/restchannel.ts
- src/common/lib/types/defaultmessage.ts
- src/common/lib/types/protocolmessage.ts
🧰 Additional context used
🪛 Biome
src/common/lib/types/message.ts
[error] 287-288: The assignment should not be in an expression.
The use of assignments in expressions is confusing.
Expressions are often considered as side-effect free.(lint/suspicious/noAssignInExpressions)
🔇 Additional comments (9)
test/support/runPlaywrightTests.js (1)
71-72
: LGTM: Clean implementation of grep configuration.The addition of the grep parameter through environment variables follows testing best practices and maintains clean code structure.
src/common/lib/types/message.ts (2)
312-312
:⚠️ Potential issueAdd type validation before type assertion.
The current implementation assumes the input values are of the correct type without validation.
- result[i] = fromValues(values[i] as Record<string, unknown>); + const value = values[i]; + if (!value || typeof value !== 'object') { + throw new ErrorInfo('Invalid message value', 40013, 400); + } + result[i] = fromValues(value as Record<string, unknown>);Likely invalid or redundant comment.
405-411
:⚠️ Potential issueUse explicit null checks in toString method.
Using truthy checks can miss zero values. Consider using explicit null checks for more accurate string representation.
- if (this.action) result += '; action=' + this.action; - if (this.serial) result += '; serial=' + this.serial; - if (this.refSerial) result += '; refSerial=' + this.refSerial; - if (this.refType) result += '; refType=' + this.refType; - if (this.updatedAt) result += '; updatedAt=' + this.updatedAt; - if (this.deletedAt) result += '; deletedAt=' + this.deletedAt; - if (this.operation) result += '; operation=' + JSON.stringify(this.operation); + if (this.action != null) result += '; action=' + this.action; + if (this.serial != null) result += '; serial=' + this.serial; + if (this.refSerial != null) result += '; refSerial=' + this.refSerial; + if (this.refType != null) result += '; refType=' + this.refType; + if (this.updatedAt != null) result += '; updatedAt=' + this.updatedAt; + if (this.deletedAt != null) result += '; deletedAt=' + this.deletedAt; + if (this.operation != null) result += '; operation=' + JSON.stringify(this.operation);Likely invalid or redundant comment.
test/realtime/crypto.test.js (1)
398-398
: LGTM: Consistent handling of message actions across encryption modes.The addition of
{ stringifyAction: true }
option in both 128-bit and 256-bit msgpack tests ensures proper stringification of the new message action field during decoding. This change aligns with the PR's objective of supporting operations in messages.Let's verify the consistency of this change across the codebase:
Also applies to: 443-443
✅ Verification successful
Let me gather more context about the
stringifyAction
option usage to ensure completeness of the verification.
The
stringifyAction
option is correctly used in the crypto testsThe verification confirms that the
stringifyAction
option is being used appropriately in the crypto tests. The codebase shows:
- The option is part of the Message class's design, converting numeric action values to their string representations using
MessageAction
enum.- It's consistently used across the codebase in message decoding scenarios:
- In protocol message handling:
messageFromValues(messages[i], { stringifyAction: true })
- In message decoding:
fromValues(encoded, { stringifyAction: true })
- In the crypto tests for both 128-bit and 256-bit encryption
The implementation in the crypto tests aligns with the established pattern for handling message actions throughout the codebase.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the usage of stringifyAction option in message decoding # Test: Search for other message decoding calls that might need the same option # Look for Message.fromValues calls ast-grep --pattern 'Message.fromValues($$$)' # Look for msgpack.decode calls rg 'msgpack\.decode'Length of output: 1304
Script:
#!/bin/bash # Search for stringifyAction usage across the codebase rg "stringifyAction" -B 2 -A 2 # Look for Message class definition to understand the option handling ast-grep --pattern 'class Message { $$$ }' # Search for message action related code rg "MessageAction" -B 2 -A 2Length of output: 16492
test/realtime/message.test.js (1)
1278-1315
: Verify the consistency of message action values.The test cases assume specific numeric values for actions (e.g., 1 for 'message.create', 2 for 'message.update'). Let's verify these values are consistent with the implementation.
✅ Verification successful
The verification results show that the message action values are defined in
src/common/lib/types/message.ts
using a zero-based index arrayMessageActionArray
. The array defines the actions in order: 'message.unset' (0), 'message.create' (1), 'message.update' (2), which perfectly matches the test cases' assumptions where 'message.create' corresponds to 1 and 'message.update' corresponds to 2.Let me verify one more aspect - the actual usage of these maps in the message implementation:
Test cases correctly reflect the message action handling implementation
The verification confirms that the test cases are consistent with the implementation:
- The action values are correctly defined in
MessageActionArray
with 'message.create' at index 1 and 'message.update' at index 2- The Message class implementation in
src/common/lib/types/message.ts
shows:
toJSON()
converts string actions to numbers usingtoMessageActionNumber
toString()
directly uses the action value (string or number)This matches the test cases' expectations where:
- Numeric action 1 maps to 'message.create'
- Numeric action 2 maps to 'message.update'
- String actions are preserved in toString() but converted to numbers in toJSON()
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify message action values are consistent across the codebase # Test: Search for action enum/constant definitions echo "Searching for message action definitions..." rg -A 5 "(?:enum|const)\s+(?:Message)?Action" # Test: Search for specific action string literals echo "Searching for action string literals..." rg -A 2 "(?:message\.create|message\.update)"Length of output: 2533
Script:
#!/bin/bash # Search for the message implementation that uses these action maps ast-grep --pattern 'class $_ { $$$ toString() { $$$ } $$$ }' # Also check for toJSON implementation ast-grep --pattern 'class $_ { $$$ toJSON() { $$$ } $$$ }'Length of output: 6443
ably.d.ts (4)
2338-2365
: LGTM: Message interface changes are well-documented and properly typed.The new fields (action, serial, refSerial, etc.) are correctly marked as optional since they may not be set when building a message, while being required in InboundMessage type. This aligns with the materialization enhancement objectives.
2435-2435
: LGTM: InboundMessage type correctly requires new fields.The addition of 'serial' and 'action' to the required fields is appropriate as these will be set on all messages across all namespaces, as confirmed in previous discussions.
2380-2383
: 🛠️ Refactor suggestionConsider using a more flexible type for metadata.
The current type
Record<string, string>
restricts metadata values to strings only. Consider usingRecord<string, unknown>
or a more flexible type to allow for structured metadata.Apply this diff to allow more flexible metadata values:
- metadata?: Record<string, string>; + metadata?: Record<string, unknown>;Likely invalid or redundant comment.
2369-2370
:⚠️ Potential issueFix typo in Operation interface comment.
The comment should read "update or deletion" instead of "update of deletion".
Apply this diff to fix the typo:
- * Contains the details of an operation, such as update of deletion, supplied by the actioning client. + * Contains the details of an operation, such as update or deletion, supplied by the actioning client.Likely invalid or redundant comment.
To support the work related to materialization, this pull request introduces new message fields and actions handling.
Added
MessageAction
enum to represent different messageactions
.Added
serial
to the message, this is the unique serial (including index) of the message.Publish now correctly sets the message action to
message_create
refSerial
refType
operation
updatedAt
deletedAt
This PR will need to wait until the related realtime work is done to support the new action fields in published messages. (ongoing)
Related to CHA-575
Summary by CodeRabbit
Release Notes
New Features
Bug Fixes
Documentation
Chores